-
Notifications
You must be signed in to change notification settings - Fork 44
Minify: Add support for deprecated directive #367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removed
Build ID: 462d8d6d1cd4f59f3f55af73 URL: https://www.apollographql.com/docs/deploy-preview/462d8d6d1cd4f59f3f55af73 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the relevant section in our docs:

https://www.apollographql.com/docs/apollo-mcp-server/define-tools#minification
… arguments if we have that present
50ad170
to
8d83382
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my feedback! 🙇
In #346, it is mentioned that our tools aren't properly picking up
deprecated
directives when building and executing operations. This led me to dig deeper and find that ourminify
handling for the builtin tools does not include any directives in the minified output. This can potentially be harmful since it means the LLM doesn't know to exclude certain fields when building operations withintrospect
.In this PR, I've added the ability to minify deprecated directives with the inclusion of their
reason
argument to give better direction to the LLM when it works within this problem space.Additionally, I've added some snapshot tests to the
minify
file so we can assert that the minified schema is built properly and includes the things we expect it to include.I've also made sure to add proper coverage for the directive since it has recently been updated to be allowed to be placed on
INPUT_FIELD_DEFINITION
andARGUMENT_DEF
(though these technically fail schema validation with our current graphql-rs parser.